Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add handler for wasm-micro-runtime (wamr) #1497

Closed
wants to merge 1 commit into from

Conversation

macko99
Copy link

@macko99 macko99 commented Jul 13, 2024

preview, not yet fully developed

Copy link

podman system tests failed. @containers/packit-build please check.

@giuseppe
Copy link
Member

thanks for working on this. Could you run a make clang-format to correctly format the code?

Also, please squash the commits into one since it is only one new feature.

@flouthoc PTAL

@macko99
Copy link
Author

macko99 commented Jul 16, 2024

@giuseppe I will have more time in 2 weeks so I will put some more love into this work, thanks for the tips!

}

// Function to read a WebAssembly binary file into a buffer
char *read_wasm_binary_to_buffer(const char *pathname, uint32_t *size) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you just use read_all_file(...) from utils.h?

src/libcrun/handlers/wamr.c Outdated Show resolved Hide resolved
src/libcrun/handlers/wamr.c Outdated Show resolved Hide resolved
fseek(file, 0, SEEK_SET);

// Allocate memory for the buffer
buffer = (char *)malloc(file_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crun uses xmalloc across the code base, i think it should exit on its own when failure happens.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giuseppe WDYT ?

error (EXIT_FAILURE, 0, "could not find wasm_runtime_set_wasi_args symbol in `libiwasm.so`");

char *buffer, error_buf[128];
uint32_t size, stack_size = 8096, heap_size = 8096;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason for choosing 8096 as stack and heap size ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flouthoc I followed this example

@flouthoc
Copy link
Collaborator

Left some comments, I will try playing with this once this is ready.

@giuseppe
Copy link
Member

please squash the commits

@macko99 macko99 marked this pull request as ready for review July 23, 2024 13:32
Signed-off-by: Maciej <[email protected]>

wamr is working!

Signed-off-by: Maciej <[email protected]>

cleanup the code

Signed-off-by: Maciej <[email protected]>

clang-format

Signed-off-by: Maciej <[email protected]>

remove unnecessary functions load

Signed-off-by: Maciej <[email protected]>

Signed-off-by: Maciej <[email protected]>

// Function to read a WebAssembly binary file into a buffer
char *
read_wasm_binary_to_buffer (const char *pathname, uint32_t *size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use read_all_file from utils.h instead of this function

// Read the file into the buffer
if (fread (buffer, 1, file_size, file) != file_size)
{
error (EXIT_FAILURE, 0, "Failed to read file");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines

      free (buffer);
      fclose (file);
      return NULL;

are never executed because EXIT_FAILURE (that is nonzero) is passed as the first argument to error().

Reference:
quote from
https://man7.org/linux/man-pages/man3/error.3.html

       If status has a nonzero value, then error() calls exit(3) to
       terminate the program using the given value as the exit status;
       otherwise it returns after printing the error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I now realised my comment about error() was unnecessary because Giuseppe
already suggested replacing the whole function.

if (! module_inst)
error (EXIT_FAILURE, 0, "Failed to instantiate the WASM module");

/* lookup a WASM function by its name; the function signature can be NULL here */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lookup
->
look up

(and the same in the error message on line 214)

if (! func || func == NULL)
error (EXIT_FAILURE, 0, "Failed to lookup the WASM function");

/* creat an execution environment to execute the WASM functions */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creat
->
create

@eriksjolund
Copy link
Contributor

eriksjolund commented Aug 14, 2024

Please edit the comment #1497 (comment) and add a new line with the text

Closes: https://github.com/containers/crun/issues/1498

This will create a link to the issue. When the PR is merged, the issue will automatically be closed.

@giuseppe
Copy link
Member

giuseppe commented Sep 9, 2024

@macko99 still working on this?

@giuseppe
Copy link
Member

giuseppe commented Oct 3, 2024

I got no feedback. Please reopen if you are still working on it

@giuseppe giuseppe closed this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants